-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Mingw cross-compilation. #2329
Conversation
+cc @brondani |
src/disk_interface_test.cc
Outdated
@@ -171,7 +171,7 @@ TEST_F(DiskInterfaceTest, StatCache) { | |||
EXPECT_GT(disk_.Stat("subdir/subsubdir", &err), 1); | |||
EXPECT_EQ("", err); | |||
|
|||
#ifndef _MSC_VER // TODO: Investigate why. Also see https://github.com/ninja-build/ninja/pull/1423 | |||
#if 0 // TODO: Investigate why. Also see https://github.com/ninja-build/ninja/pull/1423 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be #ifndef _WIN32
then, otherwise we would disable it for Linux, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll do it, but note that there is already an #ifndef _WIN32 just before this test declaration, because this is (currently) the only platform with a StatCache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see! Then we probably shouldn't touch this line at all, should we? The test succeeded on AppVeyor for MinGW: https://ci.appveyor.com/project/nico/ninja/builds/47865014/job/eg5b0uic6jgrrida
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test definitely fails when building with my Debian mingw installation, and running on a Windows 10 machine though, which is why I had to disable it there too. I have no idea why.
[3/398] MSVCHelperTest.NoReadOfStderrto stderr
[207/398] DiskInterfaceTest.StatCache
*** Failure in /work2/github-digit/ninja/src/disk_interface_test.cc:175
disk_.Stat("subdir", &err) == disk_.Stat("subdir/.", &err)
*** Failure in /work2/github-digit/ninja/src/disk_interface_test.cc:178
disk_.Stat("subdir", &err) == disk_.Stat("subdir/subsubdir/..", &err)
[398/398] BuildLogRecompactTest.Recompact
I suspect the issue is with the MSVCRT.DLL runtime being used, not the compiler that builds the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I am building with CMake, not the ./configure.py script which does not support cross-compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Still it runs fine in some cases and one of them is the CI. That's why I would like to keep it enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, we'll keep a local patch for our own CI then. I have removed the change to disk_interface_test.cc here.
0769f5a
to
00aa84f
Compare
The previous pull request broke compilation with the Mingw toolchain, which does not provide <atlcore.h>. Moreover, this massive header was only used to expand a macro that generates a static volatile variable under the hood (which is not necessary here). Replace it with with a simpler and more portable code path to fix the build.
00aa84f
to
eddafdd
Compare
Have you seen this warning?
|
I was not aware of this warning. I have pushed digit-google@f6e83fa as a work-around. Searching online for an explanation only finds a few pages that state that this is a GCC 8+ bug, without proof however. |
The previous pull request broke compilation with the Mingw toolchain, which does not provide <atlcore.h>. Moreover, this massive header was only used to expand a macro that generates a static volatile variable under the hood (which is not necessary here).
Replace it with with a simpler and more portable code path to fix the build.
Also, ensure that a broken DiskInterfaceTest, which was only disabled when using the Microsoft compiler, is also disabled for Mingw (note that the whole test code is inside a #ifdef _WIN32 .. #endif block).
After this patch, a
ninja_test.exe
generated on Linux with the Mingw toolchain runs and succeeds properly on a Windows machine.